Fix connection status issue (#2519) #3364

ann0see · 2024-09-08T09:51:25Z

This is a manual port of #2550 by @pgScorpio

Short description of changes

First step to get to know the codebase of #2550

CHANGELOG: DO NOT MERGE

Context: Fixes an issue?

Fixes: #2519
Supersedes: #2550

Does this change need documentation? What needs to be documented and how?

No

Status of this Pull Request

Ready for first review and decision on how to move forward.

What is missing until this pull request can be merged?

Review.
It compiles but not every part of the code is fully understood and commented.

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

@pgScorpio

This is a manual port of jamulussoftware#2550 by @pgScorpio Co-authored-by: ann0see <[email protected]>

ann0see · 2024-09-08T10:29:29Z

Coding style check also fails on main. Seems to be an unrelated issue?

ann0see · 2024-09-08T10:36:01Z

Pasting from previous PR here:

Moved Connect/Disconnect code from CClientdlg to CClient.
Now using the proper connected checks in several places.
Added bDisconnectAndDisable to CChannel. (For a Client now
Channel.Disconnect() will block audio data and auto disable the channel on disconnected)

src/client.cpp

src/main.cpp

src/client.cpp

ann0see · 2024-09-08T16:18:44Z

Ok.

a5af8cc is for having a smaller diff in this PR. The renaming of methods can happen in another PR.

Client.Start() and Client.Stop() have been re-introduced in 535f781, but can be - of course - merged again if wanted.

I think, this PR should now get an in depth review.

ann0see · 2024-09-08T16:20:17Z

src/client.h

@@ -121,8 +120,11 @@ class CClient : public QObject

    void Start();
    void Stop();
+    bool Connect ( QString strServerAddress, QString strServerName );


It does make sense to introduce an easy to call Connect() and Disconnect() method IMO, so this is good.

Having these in client.cpp rather than clientdlg.cpp is important. It's important enough to be done in a single PR on its own. I think we should start with that.

Agree. That's issue #3367

Factored out into #3372

ann0see · 2024-09-08T16:20:56Z

src/client.h

    bool IsRunning() { return Sound.IsRunning(); }
-    bool IsCallbackEntered() const { return Sound.IsCallbackEntered(); }
+    bool IsCallbackEntered() const { return Sound.IsCallbackEntered(); } // For OnTimerCheckAudioDeviceOk only


Reverted naming. May need some in depth review and discussion.

ann0see · 2024-09-08T16:21:50Z

src/client.h

@@ -427,8 +429,9 @@ protected slots:

    void CLChannelLevelListReceived ( CHostAddress InetAddr, CVector<uint16_t> vecLevelList );

+    void Connecting ( QString strServerName );


Probably a missing slot.

ann0see · 2024-09-08T16:22:26Z

src/client.h

    void Disconnected();
-    void SoundDeviceChanged ( QString strError );
+    void SoundDeviceChanged();


I don't feel strongly about removing the error message handling. Could be reverted.

As above:

split this out to a separate PR

have two slots, one for GUI, one for headless, and pass the error to each

ann0see · 2024-09-08T16:23:18Z

src/clientdlg.cpp

-
-        // initiate connection
-        Connect ( strSelectedAddress, strMixerBoardLabel );
+        // TODO: investigate and add error handling on failed Connect() call.


TODO as commented in code.

ann0see · 2024-09-08T16:45:09Z

I think it should only do changes on the client side.
The PR is still huge and potentially we need to extract the most important points. See the linked PR for old discussions.
My goal is to use this to lay foundations for the JSON-RPC connect/disconnect calls.

ann0see · 2024-09-08T16:45:40Z

So we need to try understanding what gets changed.

src/channel.cpp

ann0see · 2024-09-08T17:19:53Z

src/channel.h

@@ -76,7 +76,7 @@ class CChannel : public QObject

    void PrepAndSendPacket ( CHighPrioSocket* pSocket, const CVector<uint8_t>& vecbyNPacket, const int iNPacketLen );

-    void ResetTimeOutCounter() { iConTimeOut = iConTimeOutStartVal; }
+    void ResetTimeOutCounter() { iConTimeOut = bDisconnectAndDisable ? 1 : iConTimeOutStartVal; }


I don't like this. Should probably be expanded into an assignment and boolean comparison.

Yeah, I not enthused by the whole bDisconnectAndDisable -- I can't see the problem it's trying to fix.

I think they wanted to introduce an intermediate state somehow to fix the button being able to get out of sync.
From #2550 (comment)

Added bDisconnectAndDisable to CChannel. (For a Client now
Channel.Disconnect() will block audio data and auto disable the channel on disconnected)

ann0see · 2024-09-08T17:29:26Z

Maybe we can reconstruct something from pgScorpio@648880c

ann0see · 2024-09-08T17:51:10Z

src/clientdlg.cpp

@@ -1193,65 +1167,36 @@ void CClientDlg::OnCLPingTimeWithNumClientsReceived ( CHostAddress InetAddr, int
    ConnectDlg.SetPingTimeAndNumClientsResult ( InetAddr, iPingTime, iNumClients );
 }

-void CClientDlg::Connect ( const QString& strSelectedAddress, const QString& strMixerBoardLabel )
+void CClientDlg::OnConnect ( QString strServerName )


@pljones Potentially we only need this (see signal connection above) for the JSON-RPC PR.

softins · 2024-09-13T11:52:38Z

I looked quickly through the changes the other day and didn't notice anything of concern, but I haven't yet had time to study in detail.

ann0see · 2024-09-13T12:06:48Z

I believe there's a need to check if the changed conditions in the if statements are really correct or if the bug lays elsewhere

pljones · 2024-09-13T16:07:38Z

src/channel.cpp

+        }
+        else
+        {
+            // For disconnected clients set defaults


Why isn't this done when the disconnect actually happens, rather than here? Like in line 645? Is this just duplicated? Or is there a reason for it?

I think there's a reason for it.
It's probably trying to ensure that bDisconnectAndDisable is representing the client being in process of disconnecting?

I don't see what job it's doing that setting iConTimeOut to 1 doesn't already do. It still needs iConTimeOut set to 1, too, so it just adds complexity rather than removing it, without improving readability either.

pljones · 2024-09-13T16:12:33Z

src/channel.cpp

+
+            if ( bDisconnectAndDisable && !bIsServer )
+            {
+                bDisconnectAndDisable = false;


??! So these get set to the same values whether eGetStatus is GS_CHAN_NOW_DISCONNECTED or GS_CHAN_NOT_CONNECTED? Now I'm really confused.

pljones · 2024-09-13T16:14:07Z

src/channel.h

@@ -76,7 +76,7 @@ class CChannel : public QObject

    void PrepAndSendPacket ( CHighPrioSocket* pSocket, const CVector<uint8_t>& vecbyNPacket, const int iNPacketLen );

-    void ResetTimeOutCounter() { iConTimeOut = iConTimeOutStartVal; }
+    void ResetTimeOutCounter() { iConTimeOut = (bDisconnectAndDisable && !bIsServer) ? 1 : iConTimeOutStartVal; }


Isn't this what CChannel::Disconnect() is doing? Why is this change here?

The lifecycle of iConTimeOut should be pretty clear:

On client start, it should get initialised to 0, really.

On connect, and each time an audio packet is processed whilst connected, it should get set to iConTimeOutStartVal, which should be a multiple of iAudioFrameSizeSamples.

Each time no audio packet is received iAudioFrameSizeSamples will get deducted without a reset until such time time runs out and the client disconnects.

On manual disconnect, it should get set to 1, which is less than iAudioFrameSizeSamples, which means on the next time around, the client will disconnect as above.

Once disconnected, it will get set back to 0.

On manual disconnect, it should get set to 1, which is less than iAudioFrameSizeSamples, which means on the next time around, the client will disconnect as above.

Which I believe may not happen? Probably the fix is not here though.

src/client.cpp

pljones · 2024-09-13T16:27:01Z

src/client.cpp

@@ -337,7 +329,7 @@ void CClient::CreateServerJitterBufferMessage()
 void CClient::OnCLPingReceived ( CHostAddress InetAddr, int iMs )
 {
    // make sure we are running and the server address is correct
-    if ( IsRunning() && ( InetAddr == Channel.GetAddress() ) )
+    if ( Channel.IsEnabled() && ( InetAddr == Channel.GetAddress() ) )


This isn't about the channel being enabled. It's about whether the client is running. It's a different intent.

I think the initial PR confused active channel and active client: #2550 (comment)

This difference needs documentation. IMO: Add (like for JSON-RPC) respective technical documentation before the respective methods. As end goal: Write them for every function, for now: Write docs while changing.

src/client.cpp

pljones · 2024-09-13T16:37:07Z

src/client.cpp

-    Sound.Stop();
+    // start disconnection
+    // Channel.Disconnect() should automatically disable Channel as soon as disconnected.
+    // Note that this only works if sound is active!


So why make the changes here?

I believe it's mainly moving around the Channel.Disconnect() and Sound.Stop() code around.

This may be the actual bug fix?

pljones · 2024-09-13T16:43:34Z

src/clientdlg.cpp

    }
 }

-void CClientDlg::Disconnect()
+void CClientDlg::OnDisconnect()


Tidying up the mixer board should go in here, I guess.

ann0see · 2024-09-13T16:44:53Z

Thanks for the review! I'll go through what I can answer later.

src/clientsettingsdlg.cpp

pljones · 2024-09-13T17:28:53Z

src/main.cpp

@@ -963,6 +961,12 @@ int main ( int argc, char** argv )
                // show dialog
                ClientDlg.show();

+                // Connect on startup


These shouldn't be in main.cpp.

I think that's up to discussion. I do understand that one could put this functionality here. How it's handled currently also seems somewhat messy.

Yes, it is messy. But that's a bigger issue with how start up works.

Factored out into jamulussoftware#3369

ann0see · 2024-09-13T20:07:21Z

src/clientdlg.cpp

-        // disconnect the old server first
-        if ( pClient->IsRunning() )
+        // initiate connection
+        if ( pClient->Connect ( strSelectedAddress, strMixerBoardLabel ) )


This should be refactored imo. Feels very clumsy.

UI should simply pass any captured details to Client and await feedback. There shouldn't be any testing of the response. (Any input validation should be factored out of Client if it needs sharing between Client and ClientDlg.)

ann0see mentioned this pull request Sep 8, 2024

Connection status issue #2519 #2550

Closed

ann0see force-pushed the refactoring/RefactorClientConnectionUi branch 2 times, most recently from b12ce7e to 2b9d985 Compare September 8, 2024 09:58

Fix connection status issue (jamulussoftware#2519)

38fb414

This is a manual port of jamulussoftware#2550 by @pgScorpio Co-authored-by: ann0see <[email protected]>

ann0see force-pushed the refactoring/RefactorClientConnectionUi branch 2 times, most recently from d6e5a6c to 38fb414 Compare September 8, 2024 10:28

ann0see marked this pull request as draft September 8, 2024 10:34